Skip to content

editoast: add authorization to GET:rolling_stock/{id}/usage - #17383

Merged
Sh099078 merged 1 commit into
feature-branch/check-rolling-stock-privilegesfrom
lf/perm-endpoint-rs-usage
Jul 17, 2026
Merged

editoast: add authorization to GET:rolling_stock/{id}/usage#17383
Sh099078 merged 1 commit into
feature-branch/check-rolling-stock-privilegesfrom
lf/perm-endpoint-rs-usage

Conversation

@Sh099078

@Sh099078 Sh099078 commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

There are a lot of line changes but it's mainly because of tests moved into a submodule. Authorization tests have been added, the existing ones are unchanged.

Authorization rules:

  • Users must have the Reader grant on the rolling stock to get its usage.
  • No role is required (or allows) to read a rolling stock usage except admin.

Part of #17364 / https://github.com/osrd-project/osrd-confidential/issues/1064.

@Sh099078 Sh099078 self-assigned this Jun 24, 2026
@github-actions github-actions Bot added the area:editoast Work on Editoast Service label Jun 24, 2026
@Sh099078 Sh099078 moved this to In Progress in Board PI 20 Jun 24, 2026
@Sh099078 Sh099078 mentioned this pull request Jun 24, 2026
Draft
24 tasks
@Sh099078
Sh099078 force-pushed the lf/perm-endpoint-rs-usage branch 2 times, most recently from ab22a29 to 962df22 Compare June 24, 2026 20:39
@Sh099078
Sh099078 marked this pull request as ready for review June 24, 2026 20:40
@Sh099078
Sh099078 requested a review from a team as a code owner June 24, 2026 20:40
@Sh099078 Sh099078 moved this from In Progress to Awaiting merge in Board PI 20 Jun 24, 2026
younesschrifi
younesschrifi approved these changes Jun 26, 2026
View reviewed changes

@younesschrifi younesschrifi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I wrote some comments you can resolve after making the changes.

Comment thread editoast/src/views/rolling_stock.rs Outdated
Comment thread editoast/src/views/rolling_stock.rs
Comment thread editoast/src/views/rolling_stock.rs
@younesschrifi
younesschrifi force-pushed the feature-branch/check-rolling-stock-privileges branch from 9cda363 to af93581 Compare June 26, 2026 14:04
@Sh099078
Sh099078 force-pushed the lf/perm-endpoint-rs-usage branch 4 times, most recently from e8524c6 to fec34fa Compare June 29, 2026 10:22
@Sh099078
Sh099078 force-pushed the feature-branch/check-rolling-stock-privileges branch from af93581 to 49c63ff Compare June 29, 2026 14:35
@younesschrifi
younesschrifi force-pushed the feature-branch/check-rolling-stock-privileges branch from 49c63ff to 25b1202 Compare July 1, 2026 07:53
@Sh099078 Sh099078 mentioned this pull request Jul 2, 2026
Merged
@leovalais

leovalais commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

(I have pending review comments and I cannot comment on the thread unless I discard them all... thx github, this is a response to #17383 (comment))

That's indeed a serious design flaw...

I'm not a huge fan of ordering tests though.

  • Conceptually first, from authz perspective, all tests are of equal importance. Views define the relative ordering because of status codes, but that's hardly a concern of the upstream authz crate.
  • FuturesOrdered is fine, but we don't strictly reject as soon as we can. Authorization being the hot path of every incoming request, we lose nothing to be mindful about missed concurrency opportunities. (Without benchmarking, it's difficult to say if this point is relevant or not though.)
  • Some endpoints will care about which check fails first, but others will not. I think only the former should pay (performance and code complexity) for handling check relative ordering.

I've experimented a bit and came up with #17521. Lmk what you think @Sh099078. Basically no changes for endpoints where ordering is irrelevant. Handling the relative order of checks boils down to consuming the run_checks stream in an appropriate way.

@Sh099078
Sh099078 force-pushed the lf/perm-endpoint-rs-usage branch 2 times, most recently from fec34fa to 63f2e61 Compare July 2, 2026 22:14
@younesschrifi
younesschrifi force-pushed the feature-branch/check-rolling-stock-privileges branch from 25b1202 to c6e470e Compare July 3, 2026 12:33
@Sh099078 Sh099078 moved this to Awaiting merge in Board PI 21 Jul 13, 2026
@Sh099078
Sh099078 force-pushed the lf/perm-endpoint-rs-usage branch from 63f2e61 to 1f20be5 Compare July 13, 2026 10:41
@Sh099078

Sh099078 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

rebased on the feature branch

@younesschrifi
younesschrifi force-pushed the feature-branch/check-rolling-stock-privileges branch from 77c7390 to 3d24d43 Compare July 15, 2026 07:20
@Sh099078
Sh099078 force-pushed the lf/perm-endpoint-rs-usage branch from 1f20be5 to bcb7b4e Compare July 15, 2026 08:46
@Sh099078

Sh099078 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

rebase

@Sh099078

Sh099078 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

I would be in favor of merging the MR as is and keep skipping authz on the problematic test if that's fine with you @leovalais: the undefined http response code is not directly related to this PR

leovalais
leovalais approved these changes Jul 16, 2026
View reviewed changes

@leovalais leovalais left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore old comments, github won't let me delete them...

Comment thread editoast/authz/src/v2/rolling_stock.rs
Comment thread editoast/src/views/infra/mod.rs
Comment thread editoast/src/views/infra/mod.rs
Comment thread editoast/src/views/infra/mod.rs
Comment thread editoast/src/views/infra/mod.rs
Comment thread editoast/src/views/rolling_stock/light.rs
Comment thread editoast/src/views/rolling_stock.rs
@Sh099078
editoast: add authorization to GET:rolling_stock/{id}/usage
c4663a1 
- Users must have the Reader grant on the rolling stock to get its
  usage.
- No role is required (or allows) to read a rolling stock usage except
  admin.

Signed-off-by: Loup Federico <16464925+Sh099078@users.noreply.github.com>
@Sh099078
Sh099078 force-pushed the lf/perm-endpoint-rs-usage branch from 6438c47 to c4663a1 Compare July 17, 2026 12:48
@Sh099078
Sh099078 merged commit d28bc5c into feature-branch/check-rolling-stock-privileges Jul 17, 2026
31 checks passed
@Sh099078
Sh099078 deleted the lf/perm-endpoint-rs-usage branch July 17, 2026 13:32
@github-project-automation github-project-automation Bot moved this from Awaiting merge to Awaiting Validation in Board PI 21 Jul 17, 2026
@github-project-automation github-project-automation Bot moved this from Awaiting merge to Awaiting Validation in Board PI 20 Jul 17, 2026
@Maymanaf Maymanaf removed this from Board PI 20 Jul 23, 2026
@Maymanaf Maymanaf moved this from Awaiting Validation to Done in Board PI 21 Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@younesschrifi younesschrifi younesschrifi approved these changes
@leovalais leovalais leovalais approved these changes

Assignees

@Sh099078 Sh099078

Labels

area:editoast Work on Editoast Service

Projects

Board PI 21
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Sh099078 @leovalais @younesschrifi @Maymanaf